Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Quality of Life features to assume role policies. Added Find and Replace, Undo & Redo, and line wrapping for the Session Policy text field. #48

Merged
merged 8 commits into from
Jan 10, 2025

Conversation

JAEKts
Copy link
Contributor

@JAEKts JAEKts commented Nov 19, 2024

No description provided.

Added UndoManager package to the TextComponentFocusListener.java file, for undo/redo functionality. Also modified the BurpTabPanel.java file to enable Line wrapping in the assumeRoleSessionPolicyTextArea field.
Made various updates to the AWSSignerController and BurpTabPanel UI files to include find and replace functionality!
Copy link
Collaborator

@jakekarnes42 jakekarnes42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Please review the comments provide and let me know if you have any questions.

@@ -30,6 +38,35 @@ public void focusGained(FocusEvent e) {
JTextComponent textComponent = (JTextComponent) e.getComponent();
currentValue = textComponent.getText();

// Add UndoManager to the document
textComponent.getDocument().addUndoableEditListener(new UndoableEditListener() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary that we're adding these listeners every time it gains focus. If a user clicks in and out, I think that'll add multiple listeners to the component, and I'm not sure if they would conflict with each other in anyway. I might be just overthinking it, but is there a way to add the listener only once?

Copy link
Contributor Author

@JAEKts JAEKts Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this and you are completely correct. The undo/redo functionality stops working after switching text fields a lot since a bunch of listeners are added

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is still a bit off. A couple specific things:

  1. The newly added if block doesn't container the logic for the redo action. It looks like we might have close the block early.
  2. The code within the if block isn't indented.

import javax.swing.event.UndoableEditListener;
import javax.swing.AbstractAction;
import java.awt.event.ActionEvent;
import javax.swing.KeyStroke;

class TextComponentFocusListener<T extends Profile> implements FocusListener {
Copy link
Collaborator

@jakekarnes42 jakekarnes42 Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somewhat related to the comment above, but is this class the appropriate place to add this functionality?

This TextComponentFocusListener class is used across a lot of different text fields, not just the session policy text field. You can search across AWSSignerController for more examples, but here are just a few:

//Profile Region text field
view.profileRegionTextField.addFocusListener(new TextComponentFocusListener<>(this, "Region", Profile::setRegion));
//Profile Service text field
view.profileServiceTextField.addFocusListener(new TextComponentFocusListener<>(this, "Service", Profile::setService));
//Profile Key Id text field
view.profileKeyIdTextField.addFocusListener(new TextComponentFocusListener<>(this, "Key Id", Profile::setKeyId));

If I'm understanding the PR correctly, we're intending to add these functions to the Session Policy text field, but I think this will apply the functionality to lots of different text fields in the application. Can we investigate that and confirm? I might just be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, all text fields utilizing this listener will now have undo/redo capability. I haven't seen any issues with this during the testing, each text field has its own history and even after switching from text field to text field, the history is working as expected.


// Redo key strokes
KeyStroke redoKeyStroke1 = KeyStroke.getKeyStroke(KeyEvent.VK_Z, menuShortcutKeyMask | InputEvent.SHIFT_DOWN_MASK);
KeyStroke redoKeyStroke2 = KeyStroke.getKeyStroke(KeyEvent.VK_Z, menuShortcutKeyMask | InputEvent.SHIFT_DOWN_MASK);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these 2 keystrokes are the same. Maybe we wanted:

        KeyStroke redoKeyStroke1 = KeyStroke.getKeyStroke(KeyEvent.VK_Y, menuShortcutKeyMask);
        KeyStroke redoKeyStroke2 = KeyStroke.getKeyStroke(KeyEvent.VK_Z, menuShortcutKeyMask | InputEvent.SHIFT_DOWN_MASK);

That should make the 2 redo hotkeys: CTRL+Y and CTRL+SHIFT+Z (or using Command on Mac). I think that should match people's expectations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we made the right choice by reverting most of the changes in this file, but we're back to the case where a new DocumentListener is added each time the text field gains focus. I think this might still work, but I don't know if we'd run into issues like before where someone clicks in and out of the field over and over.

I think there are two routes for this class. If we want to keep the API signature the same, we could do this:

package com.netspi.awssigner.controller;

import static com.netspi.awssigner.log.LogWriter.*;
import com.netspi.awssigner.model.Profile;
import java.awt.event.FocusEvent;
import java.awt.event.FocusListener;
import java.util.Optional;
import java.util.function.BiConsumer;
import javax.swing.event.DocumentEvent;
import javax.swing.event.DocumentListener;
import javax.swing.text.JTextComponent;
import java.util.Set;
import java.util.HashSet;

class TextComponentFocusListener<T extends Profile> implements FocusListener {

    private final AWSSignerController controller;
    private final String propertyLoggingName;
    private final BiConsumer<T, String> updateFunction;
    private Optional<Profile> currentProfileOptional = Optional.empty();
    private String previousValue = "";
    private static final Set<JTextComponent> initializedTextComponents = new HashSet<>();

    public TextComponentFocusListener(AWSSignerController controller, String propertyLoggingName, BiConsumer<T, String> updateFunction) {
        this.controller = controller;
        this.propertyLoggingName = propertyLoggingName;
        this.updateFunction = updateFunction;
    }

    @Override
    public void focusGained(FocusEvent e) {
        JTextComponent textComponent = (JTextComponent) e.getComponent();
        currentProfileOptional = controller.getCurrentSelectedProfile();
        previousValue = textComponent.getText();

        // Add DocumentListener only once per component
        if (!initializedTextComponents.contains(textComponent)) {
            textComponent.getDocument().addDocumentListener(new DocumentListener() {
                @Override
                public void insertUpdate(DocumentEvent e) {
                    textChanged(textComponent);
                }

                @Override
                public void removeUpdate(DocumentEvent e) {
                    textChanged(textComponent);
                }

                @Override
                public void changedUpdate(DocumentEvent e) {
                    // Typically not used for plain text components
                }
            });
            initializedTextComponents.add(textComponent);
        }

        logDebug("Profile " + propertyLoggingName + " Text Field focus gained. Cause: " + e.getCause() + " ID:" + e.getID() + " Current value: " + previousValue);
    }

    @Override
    public void focusLost(FocusEvent e) {
        JTextComponent textComponent = (JTextComponent) e.getComponent();
        logDebug("Profile " + propertyLoggingName + " Text Field focus lost. Cause: " + e.getCause() + " ID:" + e.getID());
        String currentText = textComponent.getText();
        if (!previousValue.equals(currentText)) {
            updateProfile(currentText);
        }
    }

    private void textChanged(JTextComponent textComponent) {
        String currentText = textComponent.getText();
        if (!previousValue.equals(currentText)) {
            previousValue = currentText;
            updateProfile(currentText);
        }
    }

    private void updateProfile(String currentText) {
        if (currentProfileOptional.isPresent()) {
            Profile currentProfile = currentProfileOptional.get();
            logInfo("Profile " + currentProfile.getName() + " " + propertyLoggingName + " text changed. New Value: " + currentText);
            updateFunction.accept((T) currentProfile, currentText);
            controller.updateProfileStatus();
        } else {
            logDebug("Profile " + propertyLoggingName + " changed, but no profile selected. Ignoring.");
        }
    }
}

The above should work but the use of a Set for tracking feels like a bit of overkill. The advantage is that it's "drop-in" for the current class and wouldn't require other changes. The alternative would be to a larger change to the class like so:

package com.netspi.awssigner.controller;

import static com.netspi.awssigner.log.LogWriter.*;
import com.netspi.awssigner.model.Profile;
import java.awt.event.FocusEvent;
import java.awt.event.FocusListener;
import java.util.Optional;
import java.util.function.BiConsumer;
import javax.swing.event.DocumentEvent;
import javax.swing.event.DocumentListener;
import javax.swing.text.JTextComponent;

class TextComponentChangeListener<T extends Profile> implements FocusListener, DocumentListener {

    private final AWSSignerController controller;
    private final JTextComponent textComponent;
    private final String propertyLoggingName;
    private final BiConsumer<T, String> updateFunction;
    private String previousValue = "";

    public TextComponentChangeListener(AWSSignerController controller, JTextComponent textComponent, String propertyLoggingName, BiConsumer<T, String> updateFunction) {
        this.controller = controller;
        this.textComponent = textComponent;
        this.propertyLoggingName = propertyLoggingName;
        this.updateFunction = updateFunction;

        // Initialize previousValue and add listeners once
        this.previousValue = textComponent.getText();
        this.textComponent.addFocusListener(this);
        this.textComponent.getDocument().addDocumentListener(this);
    }

    @Override
    public void focusGained(FocusEvent e) {
        previousValue = textComponent.getText(); // Update previous value on focus gain
        logDebug("Profile " + propertyLoggingName + " Text Field focus gained. Cause: " + e.getCause() + " ID:" + e.getID() + " Current value: " + previousValue);
    }

    @Override
    public void focusLost(FocusEvent e) {
        logDebug("Profile " + propertyLoggingName + " Text Field focus lost. Cause: " + e.getCause() + " ID:" + e.getID());
        String currentText = textComponent.getText();
        if (!previousValue.equals(currentText)) {
            updateProfile(currentText);
        }
    }

    // DocumentListener methods
    @Override
    public void insertUpdate(DocumentEvent e) {
        textChanged();
    }

    @Override
    public void removeUpdate(DocumentEvent e) {
        textChanged();
    }

    @Override
    public void changedUpdate(DocumentEvent e) {
        // Typically not used for plain text components
    }

    private void textChanged() {
        String currentText = textComponent.getText();
        if (!previousValue.equals(currentText)) {
            previousValue = currentText;
            updateProfile(currentText);
        }
    }

    private void updateProfile(String currentText) {
        Optional<Profile> currentProfileOptional = controller.getCurrentSelectedProfile();
        if (currentProfileOptional.isPresent()) {
            Profile currentProfile = currentProfileOptional.get();
            try {
                @SuppressWarnings("unchecked")
                T profile = (T) currentProfile;
                logInfo("Profile " + currentProfile.getName() + " " + propertyLoggingName + " text changed. New Value: " + currentText);
                updateFunction.accept(profile, currentText);
                controller.updateProfileStatus();
            } catch (ClassCastException e) {
                logError("Type mismatch: Cannot cast " + currentProfile.getClass().getName() + " to the expected type.");
            }
        } else {
            logDebug("Profile " + propertyLoggingName + " changed, but no profile selected. Ignoring.");
        }
    }
}

I think this is a bit better because now the class is both a FocusListener and DocumentListener. We've already renamed the class to TextComponentChangeListener to reflect this. Because we're changing that class, we'd need to rename the file and update how it's used in AWSSignerController too:

private void initListeners() {
    // ...

    // Profile Region text field
    new TextComponentChangeListener<>(this, view.profileRegionTextField, "Region", Profile::setRegion);

    // Profile Service text field
    new TextComponentChangeListener<>(this, view.profileServiceTextField, "Service", Profile::setService);

    // Profile Key Id text field
    new TextComponentChangeListener<>(this, view.profileKeyIdTextField, "Key Id", Profile::setKeyId);

    // Static Credentials Access Key text field
    new TextComponentChangeListener<>(this, view.staticAccessKeyTextField, "Static Credentials Access Key", StaticCredentialsProfile::setAccessKey);

    // Static Credentials Secret Key text field
    new TextComponentChangeListener<>(this, view.staticSecretKeyTextField, "Static Credentials Secret Key", StaticCredentialsProfile::setSecretKey);

    // Static Credentials Session Token text field
    new TextComponentChangeListener<>(this, view.staticSessionTokenTextField, "Static Credentials Session Token", StaticCredentialsProfile::setSessionToken);

    // AssumeRole Role ARN text field
    new TextComponentChangeListener<>(this, view.assumeRoleRoleArnTextField, "AssumeRole Role ARN", AssumeRoleProfile::setRoleArn);

    // AssumeRole Session Name text field
    new TextComponentChangeListener<>(this, view.assumeRoleSessionNameTextField, "AssumeRole Session Name", AssumeRoleProfile::setSessionName);

    // AssumeRole External Id text field
    new TextComponentChangeListener<>(this, view.assumeRoleExternalIdTextField, "AssumeRole External Id", AssumeRoleProfile::setExternalId);

    // AssumeRole Duration text field
    new TextComponentChangeListener<>(this, view.assumeRoleDurationTextField, "AssumeRole Duration Seconds", AssumeRoleProfile::setDurationSecondsFromText);

    // AssumeRole Session Policy text area
    new TextComponentChangeListener<>(this, view.assumeRoleSessionPolicyTextArea, "AssumeRole Session Policy", AssumeRoleProfile::setSessionPolicy);

    // Command Command text field
    new TextComponentChangeListener<>(this, view.commandCommandTextField, "Command Command", CommandProfile::setCommand);

    // Command Duration text field
    new TextComponentChangeListener<>(this, view.commandDurationTextField, "Command Duration Seconds", CommandProfile::setDurationSecondsFromText);

    // ...
}

@jakekarnes42 jakekarnes42 merged commit a2e3ecf into NetSPI:master Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants